Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dictionaries "save as..." support #1244

Merged

Conversation

benoit-pierre
Copy link
Member

@benoit-pierre benoit-pierre commented Apr 10, 2021

Summary of changes

Alternative to #1149.

See commit messages for the detail of the changes.

Rational for 8dcda12, and allowing to create copies when multiple dictionaries are selected: with this PR and our support for read-only assets: resources, I think we should consider changing the system default dictionaries definition to:

DEFAULT_DICTIONARIES = (
    'user.json',
    DICTIONARIES_ROOT + '/commands.json',
    DICTIONARIES_ROOT + '/main.json',
)

This would result in:

  • only user.json is (re-)created when missing
  • the other 2 dictionaries are loaded read-only directly from the assets directory

This would make updating easier for users that don't change those dictionaries. And if you really want to edit those, you can always can now easily save a copy (and addressing #726 would provide an even better alternative).

Pull Request Checklist

  • Changes have tests
  • News fragment added in news.d.

Create a copy of each selected dictionary, or merge them into a new one.
Default to the configuration directory, instead of the current directory.
No editing or "saving as" until a dictionary is loaded.

Note: this does not strictly prevent trying to edit a loading
dictionary because the code does not filter the list of selected
dictionaries before attempting to do so. But this is use case will
result in the interface freezing until all selected dictionaries have
finished loading (and then the editor will show up).
@benoit-pierre
Copy link
Member Author

Thanks for the review @user202729! 👍

Comment on lines 388 to 398
for dictionary in dictionaries_list:
title = title_template.format(name=dictionary.short_path)
name, ext = os.path.splitext(os.path.basename(dictionary.path))
default_name = default_name_template.format(name=name)
new_filename = _get_dictionary_save_name(self, title, default_name, [ext[1:]],
initial_filename=dictionary.path)
if new_filename is None:
continue
with _new_dictionary(new_filename) as d:
d.update(self._loaded_dictionaries[dictionary.path])
need_reload = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried using this feature, and wondering if it makes sense when handling multiple dictionaries to remember the path that it was saved to.

I.e. instead of dictionary.path for initial_filename, do that for only the first, and then on subsequent dialogs open with where new_filename was pointing to.

I could definitely see wanting to make a copy of X, Y, Z dictionaries and hoping that they all end up in the same folder.

(I accidentally didn't realize that the path wasn't sticky and just tried to blindly "save" and it saved to the wrong folder)

@benoit-pierre
Copy link
Member Author

benoit-pierre commented Apr 12, 2021

Just tried using this feature, and wondering if it makes sense when handling multiple dictionaries to remember the path that it was saved to.
I.e. instead of dictionary.path for initial_filename, do that for only the first, and then on subsequent dialogs open with where new_filename was pointing to.
I could definitely see wanting to make a copy of X, Y, Z dictionaries and hoping that they all end up in the same folder.
(I accidentally didn't realize that the path wasn't sticky and just tried to blindly "save" and it saved to the wrong folder)

OK, so right know the directory is always the config folder, but instead use that for the first one, and keep the same folder for the others? What if you do another operation?

@benoit-pierre
Copy link
Member Author

It could make a lot of sense to: default to the config folder for the first invocation, and re-use the selected file directory for other invocations (including adding dictionaries).

@morinted
Copy link
Member

morinted commented Apr 12, 2021

I generally agree that caching the last-used folder would be a good idea, it would help with adding, saving, and merging dictionaries in more complex workflows.

E.g. merge two dictionaries, add it to Plover, remove the other two.

My suggestion was just how to improve the save multiple copies workflow, that takes it to the next level :)

@benoit-pierre
Copy link
Member Author

OK, let's do that.

@benoit-pierre
Copy link
Member Author

OK, done.

Default to the configuration folder, and then remember and re-use
on subsequent invocations.
@benoit-pierre benoit-pierre merged commit a66583f into openstenoproject:master Apr 12, 2021
@benoit-pierre benoit-pierre deleted the dictionaries_save_as branch April 12, 2021 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants